-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(cli): add --mode options to diff command. deprecates --change-set/--no-changeset #32830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter fails with the following errors:
❌ Features must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/32830/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
0a38fbc to
c0bf4db
Compare
|
Clarification Request: Integration test has been added but the linter still doesn't seems too happy. Anything I can do about that? |
c0bf4db to
8e9f761
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting started on this!
In stead of introducing a new boolean option that only sometimes has an effect, we should create a new 3-way switch option to replace the current --change-set and --no-change-set options.
Something like
--mode=auto
--mode=change-set
--mode=template-only
8e9f761 to
e7a536b
Compare
e7a536b to
95be843
Compare
|
@mrgrain I have addressed your feedback. Let me know what you reckon |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
@msessa FYI, we've parked this to prioritize some other work in the CLI area. I'll be getting back it end of February. Apologies for the delay. |
|
Apologies for the delay and lack of update. This is still on our list but needs to be pushed back further. Will give another update in May. |
The primary motivation for this PR is https://github.com/aws/aws-cdk/issues/28753 and the ensuing PR aws/aws-cdk#32830. When implementing diff in the programmatic toolkit library we included the diff method options, and this PR adds the `fallBackToTemplate` property to the `ChangeSet` mode. The idea is that people can specify `fallBackToTemplate=false` to explicitly fail if we cannot successfully create the changeset. The current behavior is to fallback to the template-only method. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
|
Hi @msessa apologies, this really slipped off my plate. The cli repo and toolkit-lib are now in a good state to take contributions again. You can find the repo here: https://github.com/aws/aws-cdk-cli If you are still interested, feel free to re-create the PR on the cli repo. We will then review and handle it over there. Otherwise I have re-opened and transferred the original issue over to the new repo and we are tracking and prioritizing it accordingly. |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes aws/aws-cdk-cli#914
Reason for this change
The current behaviour of implicitly reverting to template-only diff can hide important error messages from the user that can help catching mistakes early before deployment.
This is especially true when a template uses transforms or when using changeset-level cloudformation hooks to enforce compliance rules.
See an example code snippet at the bottom
Description of changes
Added a
--modeoption to thediffcommand that replaces (deprecates)--change-set/--no-changeset.The following modes are supported:
auto: Attempts changeset creation and fallback tolocalmode should any error be encountered. (replaces-change-set)change-set: Attempts changeset creation but doesn't handle errors returned by cloudformation when creating a changeset for an existing stack. Instead those errors are surfaced to the usertemplate-only: Uses template-only diff (replaces--no-change-set)Description of how you validated changes
Checklist
Example Code
Current behaviour on diff:
New behaviour on diff:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license